fix(windows): add Node.js path detection for claude.cmd validation #1277
Conversation
Fixes issue where Windows users with npm-installed Claude CLI (claude.cmd) couldn't be detected because Node.js wasn't in PATH. Added automatic Node.js directory detection and PATH augmentation when validating .cmd files. - Added findNodeJsDirectories() in platform/paths.ts - Updated Claude CLI validation in cli-tool-manager.ts - Updated claude-code-handlers.ts validation - Updated claude-cli-utils.ts to include Node.js in PATH Resolves claude.cmd requiring node.exe to execute on Windows. Signed-off-by: JTnv <[email protected]>
|
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughWindows-specific Node.js directory discovery was added and PATH-augmentation for Claude CLI validation/invocation was refactored: callers now collect the CLI command directory plus discovered Node.js dirs (when validating a Changes
Sequence Diagram(s)sequenceDiagram
participant Validator as "Claude CLI Validator"
participant Platform as "platform.findNodeJsDirectories()"
participant Env as "getAugmentedEnv()"
participant Proc as "Spawn Process"
Validator->>Platform: request Node.js dirs (Windows-only)
Platform-->>Validator: list of candidate dirs (maybe [])
Validator->>Env: call with [cliDir, ...nodeDirs] (filtered)
Env-->>Validator: augmented env (PATH with prepended entries)
Validator->>Proc: spawn validation process with augmented env
Proc-->>Validator: validation result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @JTnv, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Claude CLI's compatibility and user experience on Windows by addressing a critical issue where the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thanks for your first PR!
A maintainer will review it soon. Please make sure:
- Your branch is synced with
develop - CI checks pass
- You've followed our contribution guide
Welcome to the Auto Claude community!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a fix for Windows users by automatically detecting Node.js installations and augmenting the PATH when validating claude.cmd. This is a valuable improvement for the Windows user experience. My review focuses on enhancing the robustness and maintainability of the implementation. I've suggested using environment variables instead of hardcoded paths, consolidating duplicated logic, optimizing path-checking performance, and correcting a minor issue in PATH string construction. Overall, the changes are solid and address the problem effectively.
| for (const nodeDir of nodeDirs) { | ||
| const normalizedNodeDir = path.normalize(nodeDir); | ||
| const hasNodeDir = pathEntries | ||
| .map((entry) => path.normalize(entry).toLowerCase()) | ||
| .includes(normalizedNodeDir.toLowerCase()); | ||
| if (!hasNodeDir) { | ||
| dirsToAdd.push(nodeDir); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation iterates through nodeDirs and for each directory, it maps over pathEntries to check for existence. This can be inefficient if the PATH environment variable is long, leading to a time complexity of O(M*N). A more performant approach would be to create a Set of normalized path entries once, and then check each Node.js directory against the set. This reduces the complexity to O(M+N).
| for (const nodeDir of nodeDirs) { | |
| const normalizedNodeDir = path.normalize(nodeDir); | |
| const hasNodeDir = pathEntries | |
| .map((entry) => path.normalize(entry).toLowerCase()) | |
| .includes(normalizedNodeDir.toLowerCase()); | |
| if (!hasNodeDir) { | |
| dirsToAdd.push(nodeDir); | |
| } | |
| } | |
| const normalizedPathEntries = new Set( | |
| pathEntries.map((entry) => path.normalize(entry).toLowerCase()) | |
| ); | |
| for (const nodeDir of nodeDirs) { | |
| const normalizedNodeDir = path.normalize(nodeDir).toLowerCase(); | |
| if (!normalizedPathEntries.has(normalizedNodeDir)) { | |
| dirsToAdd.push(nodeDir); | |
| } | |
| } |
| // On Windows, if validating claude.cmd, also add Node.js directories to PATH | ||
| // This is needed because claude.cmd requires node.exe to execute | ||
| if (isWindows() && /\.cmd$/i.test(unquotedCmd)) { | ||
| const nodeDirs = findNodeJsDirectories(); | ||
| additionalPaths = [...additionalPaths, ...nodeDirs]; | ||
| console.warn('[Claude CLI] Adding Node.js directories to PATH for .cmd validation:', nodeDirs); | ||
| } |
There was a problem hiding this comment.
This logic for adding Node.js directories to the path for .cmd files on Windows is duplicated in validateClaudeAsync (lines 1100-1106). To improve maintainability and reduce code duplication, this logic could be extracted into a private helper function. This function could then be called from both validateClaude and validateClaudeAsync.
Here is an example of what that helper function could look like:
private getAdditionalPathsForClaudeCmd(unquotedCmd: string, basePaths: string[]): string[] {
let additionalPaths = [...basePaths];
if (isWindows() && /\.cmd$/i.test(unquotedCmd)) {
const nodeDirs = findNodeJsDirectories();
additionalPaths.push(...nodeDirs);
console.warn('[Claude CLI] Adding Node.js directories to PATH for .cmd validation:', nodeDirs);
}
return additionalPaths;
}| const env = { | ||
| ...process.env, | ||
| PATH: cliDir ? `${cliDir}${path.delimiter}${process.env.PATH || ''}` : process.env.PATH, | ||
| PATH: pathEntries.filter(Boolean).concat(process.env.PATH || '').join(path.delimiter), |
There was a problem hiding this comment.
The construction of the PATH variable can be improved for clarity and to avoid a potential trailing delimiter. If process.env.PATH is empty or undefined, concat will add an empty string to the array, which join might turn into a trailing path separator. A cleaner way to construct the path is to use spread syntax and filter out all empty entries before joining, similar to the pattern used in claude-cli-utils.ts.
| PATH: pathEntries.filter(Boolean).concat(process.env.PATH || '').join(path.delimiter), | |
| PATH: [...pathEntries, process.env.PATH || ''].filter(Boolean).join(path.delimiter), |
| joinPaths('C:\\Program Files', 'nodejs'), | ||
| joinPaths('C:\\Program Files (x86)', 'nodejs'), | ||
|
|
||
| // User-level npm global directory (may contain node.exe with nvm-windows) | ||
| joinPaths(homeDir, 'AppData', 'Roaming', 'npm'), | ||
|
|
||
| // NVM for Windows default location | ||
| joinPaths(homeDir, 'AppData', 'Roaming', 'nvm'), | ||
| joinPaths('C:\\Program Files', 'nvm'), | ||
|
|
||
| // Scoop installation | ||
| joinPaths(homeDir, 'scoop', 'apps', 'nodejs', 'current'), | ||
|
|
||
| // Chocolatey installation | ||
| joinPaths('C:\\ProgramData', 'chocolatey', 'bin'), |
There was a problem hiding this comment.
This function uses hardcoded paths for Program Files, Program Files (x86), AppData, and ProgramData. This can be brittle if Windows is installed on a different drive or if these folders are relocated. For better robustness and consistency with other parts of the codebase (e.g., getPythonPaths), you should use the corresponding environment variables (process.env.ProgramFiles, process.env['ProgramFiles(x86)'], process.env.APPDATA, process.env.ProgramData) with fallbacks.
| joinPaths('C:\\Program Files', 'nodejs'), | |
| joinPaths('C:\\Program Files (x86)', 'nodejs'), | |
| // User-level npm global directory (may contain node.exe with nvm-windows) | |
| joinPaths(homeDir, 'AppData', 'Roaming', 'npm'), | |
| // NVM for Windows default location | |
| joinPaths(homeDir, 'AppData', 'Roaming', 'nvm'), | |
| joinPaths('C:\\Program Files', 'nvm'), | |
| // Scoop installation | |
| joinPaths(homeDir, 'scoop', 'apps', 'nodejs', 'current'), | |
| // Chocolatey installation | |
| joinPaths('C:\\ProgramData', 'chocolatey', 'bin'), | |
| joinPaths(process.env.ProgramFiles || 'C:\\Program Files', 'nodejs'), | |
| joinPaths(process.env['ProgramFiles(x86)'] || 'C:\\Program Files (x86)', 'nodejs'), | |
| // User-level npm global directory (may contain node.exe with nvm-windows) | |
| joinPaths(process.env.APPDATA || path.join(homeDir, 'AppData', 'Roaming'), 'npm'), | |
| // NVM for Windows default location | |
| joinPaths(process.env.APPDATA || path.join(homeDir, 'AppData', 'Roaming'), 'nvm'), | |
| joinPaths(process.env.ProgramFiles || 'C:\\Program Files', 'nvm'), | |
| // Scoop installation | |
| joinPaths(homeDir, 'scoop', 'apps', 'nodejs', 'current'), | |
| // Chocolatey installation | |
| joinPaths(process.env.ProgramData || 'C:\\ProgramData', 'chocolatey', 'bin'), |
| .sort((a, b) => { | ||
| // Sort by version descending (newest first) | ||
| const vA = a.name.slice(1).split('.').map(Number); | ||
| const vB = b.name.slice(1).split('.').map(Number); | ||
| for (let i = 0; i < 3; i++) { | ||
| const diff = (vB[i] ?? 0) - (vA[i] ?? 0); | ||
| if (diff !== 0) return diff; | ||
| } | ||
| return 0; | ||
| }) |
There was a problem hiding this comment.
This custom version sorting logic is also present in sortNvmVersionDirs in cli-tool-manager.ts. This code duplication can lead to inconsistencies. Furthermore, the semver package is already a dependency in this project and is more robust for version comparisons, as it correctly handles a wider range of version formats.
I suggest importing semver and using semver.rcompare to simplify and unify the version sorting. You will need to add import semver from 'semver'; to the top of the file.
.sort((a, b) => semver.rcompare(a.name, b.name))![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/main/ipc-handlers/claude-code-handlers.ts (1)
40-40: UseisWindows()from the platform module instead of directprocess.platformcheck.Per coding guidelines, direct
process.platformchecks should be avoided. Import and useisWindows()from the platform module for consistency.Suggested fix
Add to imports at line 22:
-import { findNodeJsDirectories } from '../platform'; +import { findNodeJsDirectories, isWindows } from '../platform';Then update line 40:
- const isWindows = process.platform === 'win32'; + const isWindowsPlatform = isWindows();And update references (lines 43, 53, 69) to use
isWindowsPlatform.Based on learnings, the coding guidelines state: "Do not check
process.platformdirectly in code - always import platform detection functions from the platform abstraction module."
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/claude-cli-utils.ts`:
- Line 34: Replace the direct platform check that uses process.platform in
claude-cli-utils.ts with the platform abstraction: import and use isWindows()
from the platform module and update the conditional that currently reads the
process.platform check to use isWindows() (i.e., change the if that checks
process.platform === 'win32' && /\.cmd$/i.test(command) to use isWindows() &&
/\.cmd$/i.test(command)); ensure the new import is added at the top of the file
so the function where this conditional appears uses the platform abstraction
consistently.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🔴 BLOCKED
🔴 Blocked - 4 CI check(s) failing. Fix CI before merge.
Blocked: 4 CI check(s) failing. Fix CI before merge.
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- CI Failed: CI Complete
- CI Failed: test-frontend (macos-latest)
- CI Failed: test-frontend (ubuntu-latest)
- CI Failed: test-frontend (windows-latest)
Findings Summary
- Medium: 3 issue(s)
- Low: 1 issue(s)
Generated by Auto Claude PR Review
Findings (4 selected of 4 total)
🟡 [1f54828cd11b] [MEDIUM] Direct process.platform checks violate CLAUDE.md guidelines
📁 apps/frontend/src/main/claude-cli-utils.ts:34
The file uses process.platform === 'win32' directly in 3 locations (lines 16, 21, 34) instead of using the platform abstraction module. CLAUDE.md explicitly states: 'Import from these modules instead of checking process.platform directly' and shows this exact pattern as the WRONG approach. The file already imports from './platform' but only imports findNodeJsDirectories, not isWindows() or getPathDelimiter().
Suggested fix:
Import `isWindows` and `getPathDelimiter` from './platform' and replace all direct process.platform checks:
- Line 16: Use `getPathDelimiter()` instead of ternary
- Lines 21, 34: Use `isWindows()` instead of `process.platform === 'win32'`
🟡 [e9a862d64cae] [MEDIUM] Node.js path augmentation logic duplicated in 4 locations
📁 apps/frontend/src/main/cli-tool-manager.ts:948
The same pattern for detecting Windows .cmd files and adding Node.js directories to PATH is duplicated in 4 locations: (1) claude-cli-utils.ts:34-46, (2) cli-tool-manager.ts:948-956 (validateClaude), (3) cli-tool-manager.ts:1099-1107 (validateClaudeAsync), (4) claude-code-handlers.ts:51-59. Each repeats the same Windows check, findNodeJsDirectories() call, and PATH augmentation logic.
Suggested fix:
Extract a shared helper function like `augmentPathWithNodeDirsForCmd(env: Record<string, string>, cmdPath: string): Record<string, string>` in env-utils.ts or platform module. All 4 locations would then call this single function.
🟡 [c0bc218b2492] [MEDIUM] Version sorting logic duplicates existing sortNvmVersionDirs function
📁 apps/frontend/src/main/platform/paths.ts:211
The version sorting logic in findNodeJsDirectories() (lines 211-223) reimplements the exact same semver sorting algorithm that already exists as the exported sortNvmVersionDirs() function in cli-tool-manager.ts (lines 207-228). Both use the same regex /^v\d+\.\d+\.\d+$/, the same 3-part version comparison loop, and produce the same output.
Suggested fix:
Import sortNvmVersionDirs from cli-tool-manager.ts and reuse it: `const versionDirs = sortNvmVersionDirs(entries).map(name => joinPaths(nvmPath, name))`
🔵 [1e36f4225ffd] [LOW] Silent error swallowing when reading NVM directory
📁 apps/frontend/src/main/platform/paths.ts:226
The catch block on lines 226-228 in findNodeJsDirectories() silently ignores errors when reading the NVM directory. While fault tolerance is appropriate here, there is no logging to indicate an error occurred, making debugging harder when users report Node.js detection issues.
Suggested fix:
Add a console.warn inside the catch block: `catch (error) { console.warn('[findNodeJsDirectories] Failed to read NVM directory:', error); }`
This review was generated by Auto Claude.
- Replace direct process.platform checks with platform abstractions (isWindows, getPathDelimiter) - Simplify Node.js path augmentation to match pattern used in cli-tool-manager - Reuse existing sortNvmVersionDirs function instead of duplicating version sorting logic - Add error logging to findNodeJsDirectories for better debugging Fixes all 4 medium/low severity issues identified in PR review. Signed-off-by: JTnv <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/main/platform/paths.ts (1)
12-214: MovesortNvmVersionDirsto a shared platform utility to break the circular dependency.Circular dependency confirmed:
cli-tool-manager.tsimports from./platform, whileplatform/paths.tsimportssortNvmVersionDirsfrom../cli-tool-manager. This bidirectional dependency can cause module ordering issues at runtime, especially in Electron's main process.The
sortNvmVersionDirsfunction is self-contained (no dependencies on cli-tool-manager) and should be moved toplatform/as a utility. This will keep the import graph acyclic: cli-tool-manager can import platform utilities, and platform utilities won't need to import from cli-tool-manager.
♻️ Duplicate comments (2)
apps/frontend/src/main/platform/paths.ts (1)
186-223: Avoid hardcoded Windows install paths in Node.js discovery.This function hardcodes
C:\Program Files,C:\Program Files (x86), andC:\ProgramData. That’s brittle on systems where Windows lives on a different drive or folders are relocated. Prefer environment variables and only add candidates when the env var exists; also useNVM_HOMEfor the NVM scan path so relocated installs are handled.🔧 Proposed fix
- const candidates: string[] = [ - // Standard Node.js installer location - joinPaths('C:\\Program Files', 'nodejs'), - joinPaths('C:\\Program Files (x86)', 'nodejs'), - - // User-level npm global directory (may contain node.exe with nvm-windows) - joinPaths(homeDir, 'AppData', 'Roaming', 'npm'), - - // NVM for Windows default location - joinPaths(homeDir, 'AppData', 'Roaming', 'nvm'), - joinPaths('C:\\Program Files', 'nvm'), - - // Scoop installation - joinPaths(homeDir, 'scoop', 'apps', 'nodejs', 'current'), - - // Chocolatey installation - joinPaths('C:\\ProgramData', 'chocolatey', 'bin'), - ]; + const programFiles = process.env.ProgramFiles; + const programFilesX86 = process.env['ProgramFiles(x86)']; + const appData = process.env.APPDATA; + const programData = process.env.ProgramData; + const nvmHome = process.env.NVM_HOME; + + const candidates: string[] = []; + if (programFiles) candidates.push(joinPaths(programFiles, 'nodejs'), joinPaths(programFiles, 'nvm')); + if (programFilesX86) candidates.push(joinPaths(programFilesX86, 'nodejs')); + if (appData) candidates.push(joinPaths(appData, 'npm'), joinPaths(appData, 'nvm')); + if (nvmHome) candidates.push(nvmHome); + candidates.push(joinPaths(homeDir, 'scoop', 'apps', 'nodejs', 'current')); + if (programData) candidates.push(joinPaths(programData, 'chocolatey', 'bin'));- const nvmPath = joinPaths(homeDir, 'AppData', 'Roaming', 'nvm'); - if (existsSync(nvmPath)) { + const nvmPath = nvmHome ?? (appData ? joinPaths(appData, 'nvm') : undefined); + if (nvmPath && existsSync(nvmPath)) {Based on learnings, avoid hardcoded platform-specific paths.
apps/frontend/src/main/claude-cli-utils.ts (1)
16-54: Precompute normalized PATH entries to avoid O(M×N) scans.
pathEntriesis normalized repeatedly for each directory, which is avoidable. Use a Set of normalized entries once and reuse it.♻️ Proposed refactor
- const pathEntries = currentPath.split(pathSeparator); - const normalizedCommandDir = path.normalize(commandDir); - const hasCommandDir = isWindows() - ? pathEntries - .map((entry) => path.normalize(entry).toLowerCase()) - .includes(normalizedCommandDir.toLowerCase()) - : pathEntries - .map((entry) => path.normalize(entry)) - .includes(normalizedCommandDir); + const pathEntries = currentPath.split(pathSeparator); + const normalizedPathEntries = new Set( + pathEntries.map((entry) => { + const normalized = path.normalize(entry); + return isWindows() ? normalized.toLowerCase() : normalized; + }) + ); + const normalizedCommandDir = path.normalize(commandDir); + const normalizedCommandDirKey = isWindows() + ? normalizedCommandDir.toLowerCase() + : normalizedCommandDir; + const hasCommandDir = normalizedPathEntries.has(normalizedCommandDirKey); ... - const pathEntriesToAdd = dirsToAdd.filter((dir) => { - const normalizedDir = path.normalize(dir); - if (isWindows()) { - return !pathEntries - .map((entry) => path.normalize(entry).toLowerCase()) - .includes(normalizedDir.toLowerCase()); - } - return !pathEntries - .map((entry) => path.normalize(entry)) - .includes(normalizedDir); - }); + const pathEntriesToAdd = dirsToAdd.filter((dir) => { + const normalizedDir = path.normalize(dir); + const key = isWindows() ? normalizedDir.toLowerCase() : normalizedDir; + return !normalizedPathEntries.has(key); + });
- Add platform module mock to properly simulate Windows on non-Windows CI runners - Fix test mocks to be more specific, preventing detection order issues - Replace hardcoded Windows paths with environment variables in findNodeJsDirectories() - Extract getAdditionalPathsForValidation() helper to eliminate code duplication - Update test expectations to match actual security check implementation Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/__tests__/cli-tool-manager.test.ts`:
- Around line 172-189: The platform boolean mocks (mockIsWindows, mockIsMacOS,
mockIsUnix) are not reset by vi.clearAllMocks(), so add a small helper (e.g.,
resetPlatformMocks) that calls mockIsWindows.mockReturnValue(false),
mockIsMacOS.mockReturnValue(false), mockIsUnix.mockReturnValue(true) (or
whatever known default you want) and invoke that helper in the beforeEach of
both the Windows sync and Windows async describe blocks so each describe starts
with a deterministic platform state; reference the existing mock names
(mockIsWindows, mockIsMacOS, mockIsUnix) and replace any retained behavior from
prior tests rather than relying solely on vi.clearAllMocks().
In `@apps/frontend/src/main/platform/paths.ts`:
- Line 12: Importing sortNvmVersionDirs from cli-tool-manager introduces a
layering/circular dependency; move the sortNvmVersionDirs implementation into a
small shared util module (or into the platform module) and update imports: add
the new util exporting sortNvmVersionDirs, change the import in the platform
code that currently references sortNvmVersionDirs to import from the new util,
and update cli-tool-manager to import the helper from the same new location so
cli-tool-manager no longer depends on platform, breaking the cycle.
- Around line 186-210: The code currently uses hardcoded Windows fallbacks
(e.g., 'C:\\Program Files') when setting programFiles, programFilesX86, appData,
and programData; remove those fallbacks and only use the environment variables,
then build the candidates array conditionally (push joinPaths(...) entries only
if the corresponding env var is defined). Specifically, change
programFiles/programFilesX86/appData/programData to read directly from
process.env (no 'C:\\...' defaults), and construct candidates by checking each
env var before adding joinPaths(programFiles, 'nodejs'),
joinPaths(programFilesX86, 'nodejs'), joinPaths(appData, 'npm'),
joinPaths(appData, 'nvm'), and joinPaths(programData, 'chocolatey', 'bin'); keep
entries that don’t rely on hardcoded paths (e.g., scoop using homeDir)
unchanged. Ensure references: homeDir, programFiles, programFilesX86, appData,
programData, candidates, joinPaths.
- Fix circular dependency: move sortNvmVersionDirs to platform/index.ts and re-export from cli-tool-manager for backwards compatibility - Remove hardcoded path fallbacks in findNodeJsDirectories() (only use paths when environment variables are defined) - Optimize PATH lookup with Set for O(1) lookups instead of O(n) array includes - Add explicit platform mock reset in test beforeEach hooks - Include sortNvmVersionDirs from original in platform mock for tests Co-Authored-By: Claude Opus 4.5 <[email protected]>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| try { | ||
| // Find all version directories (e.g., v20.0.0, v18.17.1) | ||
| const entries = readdirSync(nvmPath, { withFileTypes: true }); | ||
| const versionNames = sortNvmVersionDirs(entries); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Move sortNvmVersionDirs definition to paths.ts and re-export from index.ts. This breaks the circular dependency where: - index.ts exported from paths.ts - paths.ts imported sortNvmVersionDirs from index.ts Now sortNvmVersionDirs lives in paths.ts alongside findNodeJsDirectories which uses it, and index.ts re-exports it for backwards compatibility. Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/platform/paths.ts`:
- Line 11: Update the Node/NVM discovery logic to honor NVM_HOME and NVM_SYMLINK
by including process.env.NVM_HOME and process.env.NVM_SYMLINK when constructing
candidate paths for node executables (same change where isWindows,
getExecutableExtension, joinPaths and sortNvmVersionDirs are used);
specifically, when scanning NVM/appdata locations on Windows add the NVM_HOME
bin and the NVM_SYMLINK location as high-priority candidates (apply the same
additions to the other NVM scan range around lines 185-235) so node.exe is found
for custom NVM installs.
Add support for NVM for Windows environment variables: - NVM_SYMLINK: Points directly to active Node.js version (highest priority) - NVM_HOME: Custom NVM installation directory (falls back to APPDATA/nvm) This improves Node.js detection coverage for users with non-default NVM installations. Co-Authored-By: Claude Opus 4.5 <[email protected]>
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🔴 Follow-up Review: Blocked
🔴 Blocked - 4 CI check(s) failing. Fix CI before merge.
Resolution Status
- ✅ Resolved: 3 previous findings addressed
- ❌ Unresolved: 1 previous findings remain
- 🆕 New Issues: 2 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 0 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 2 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- Branch Out of Date: PR branch is behind the base branch and needs to be updated
Verdict
BLOCKED: 4 CI checks failing (CI Complete, test-frontend on macos-latest, ubuntu-latest, windows-latest). The code changes successfully address 3 of 4 previous findings (platform abstraction, reusing sortNvmVersionDirs, error logging). The remaining partial resolution (duplication) and new findings (circular import) are LOW severity quality issues, not blocking. However, CI MUST pass before merge. Fix the failing frontend tests first.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (2 selected of 2 total)
🔵 [NEW-001] [LOW] Circular import between platform/paths.ts and cli-tool-manager.ts
📁 apps/frontend/src/main/platform/paths.ts:12
A circular dependency exists: paths.ts imports sortNvmVersionDirs from cli-tool-manager.ts, while cli-tool-manager.ts imports from platform (which re-exports from paths.ts). This works at runtime but is architectural debt that should be addressed by moving sortNvmVersionDirs to a shared utility.
Suggested fix:
Move sortNvmVersionDirs to platform/nvm-utils.ts to break the circular dependency
🔵 [NEW-002] [LOW] NVM directory path listed twice in candidates array
📁 apps/frontend/src/main/platform/paths.ts:196
The path AppData/Roaming/nvm is listed both in the static candidates array (line 196) and used again in the NVM version enumeration block (line 207), resulting in redundant existence checks.
Suggested fix:
Remove the duplicate NVM path from the initial candidates array since it's handled in the version enumeration block
This review was generated by Auto Claude.
3 participants
Problem
Windows users with Claude CLI installed via npm (as
claude.cmd) experience detection failures becauseclaude.cmdrequires Node.js to be in PATH to execute.Solution
Added automatic Node.js directory detection on Windows and augmented PATH when validating/running
.cmdfiles.Changes:
findNodeJsDirectories()to detect Node.js installations (standard installer, NVM, Scoop, Chocolatey).cmdfilesTesting
Tested on Windows 11 with Claude CLI installed via
npm install -g @anthropic-ai/claude-codeBefore: Claude CLI not detected,
node is not recognizederrorsAfter: Claude CLI properly detected and validated
Technical Details
The fix detects Node.js installations from:
C:\Program Files\nodejs)%APPDATA%\npm)When validating
claude.cmd, the fix automatically adds these directories to PATH sonode.execan be found.This improves Windows user experience significantly, eliminating a common setup pain point.
Summary by CodeRabbit
Bug Fixes
Other
Tests
✏️ Tip: You can customize this high-level summary in your review settings.